Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useTrackedState from reactive-react-redux #1503

Closed
wants to merge 10 commits into from

Conversation

dai-shi
Copy link
Contributor

@dai-shi dai-shi commented Jan 20, 2020

For #1502.

This brings useTrackedState from reactive-react-redux and adjusts for react-redux store context.

TODOs:

  • check tree-shakable
  • custom context support
  • spec for useTrackedState
  • docs

@netlify
Copy link

netlify bot commented Jan 20, 2020

Deploy preview for react-redux-docs ready!

Built with commit 7d20db4

https://deploy-preview-1503--react-redux-docs.netlify.app

@markerikson
Copy link
Contributor

markerikson commented Jan 20, 2020

Cool.

Primary points of concern:

  • Does this get properly tree-shaken in typical CRA / RN setups if it isn't used?
  • We'll need documentation:
    • How does this get used?
    • Why would you want to use it?
    • What are the differences in behavior compared to useSelector?
    • What are the limitations in browser support?

@dai-shi
Copy link
Contributor Author

dai-shi commented Jan 21, 2020

tree-shaken

I made a simple counter + textbox app and built with CRA.

useTrackedState + useSelector

wc build/static/js/*.js
       2    2847  140002 build/static/js/2.14348f2c.chunk.js
       1     143    7306 build/static/js/main.592541a8.chunk.js
       1      37    1569 build/static/js/runtime-main.3ed87fa7.js
       4    3027  148877 total

useSelector only

wc build/static/js/*.js
       2    2847  140002 build/static/js/2.14348f2c.chunk.js
       1     103    5171 build/static/js/main.0b18769f.chunk.js
       1      37    1569 build/static/js/runtime-main.3ed87fa7.js
       4    2987  146742 total

useTrackedState only

wc build/static/js/*.js
       2    2847  140002 build/static/js/2.14348f2c.chunk.js
       1     114    6405 build/static/js/main.dbc372e9.chunk.js
       1      37    1569 build/static/js/runtime-main.3ed87fa7.js
       4    2998  147976 total

@dai-shi
Copy link
Contributor Author

dai-shi commented Jan 26, 2020

I'm not super good at documentation. Would appreciate for any help (not only for contents but also for grammar.)

@dai-shi dai-shi marked this pull request as ready for review January 26, 2020 06:38
cache: new WeakMap()
}
})
useIsomorphicLayoutEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's not an immediate concern, I'm noticing that we now have 3 different parts of our codebase where we're subscribing to the store in a hook. That may be worth looking at to see if there's some way to abstract it.

docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
docs/api/proxy-based-tracking.md Outdated Show resolved Hide resolved
@dai-shi
Copy link
Contributor Author

dai-shi commented Feb 4, 2020

This file is not currently added to the Docusaurus sidebar, and so it's not showing up in the docs.

Having said that: for now, let's put the content into the existing "Hooks" docs page.

I think I did it so, but it's not showing in the preview. Is anything wrong?

@joelmoss
Copy link

Love this! My only concern is there seems to be too many caveats, and holes in which to fall through if not fully aware of how it works.

@dai-shi
Copy link
Contributor Author

dai-shi commented Feb 19, 2020

I understand your concern. There are some ideas to mitigate caveats, but they are not free either. My question would be how big those holes are in real world apps.

@joelmoss
Copy link

For me, I think it would be all worth it, but any time there is a hole, it is inevitable that someone will fall in, and not take that fall well at all 😕

@dai-shi how have users of reactive-react-redux handled this? Have you experienced many reports or complaints about the caveats?

@dai-shi
Copy link
Contributor Author

dai-shi commented Feb 19, 2020

There are not many users of reactive-react-redux, so I can't tell. That's the point of having this in react-redux.
Those caveats are more or less theoretical, and someone expressed concerns but not real issues.
But for the second caveat, I personally hit it. I was so shocked to find it out.
The third one is hypothetical. It may only slow down a bit. Or, it may behave unexpectedly in some middleware.
It would be nice if we could detect falling into the hole. Yeah, that might be worth considering.

@joelmoss
Copy link

It would be nice if we could detect falling into the hole. Yeah, that might be worth considering.

Yeah, think that would be a good idea.

The first caveat should be easy to track and to throw an error/warning. Not sure about the others, but if not possible to track and warn, then maybe create a few eslint rules?

@dai-shi
Copy link
Contributor Author

dai-shi commented Feb 19, 2020

The first one is actually difficult if you consider custom hooks. can eslint rules detect it?

For the second one, I have an idea with heuristics, only effective in DEV. I'll tackle this first.

For the third one, we have several options: a) allow leaking proxies and deproxify before wrapping with a new proxy, b) disallow it and detect it in DEV before wrapping with a new proxy, c) disallow it and detect it in DEV with a patched dispatch, d) deproxify always with a patched dispatch. Of course, dispatch is not only the place to leak proxies, but it should be a typical path.

@dai-shi
Copy link
Contributor Author

dai-shi commented Feb 20, 2020

For the second one, I have an idea with heuristics, only effective in DEV. I'll tackle this first.

I was too naive. My first trial reveals that it comes with false positives quite easily. I don't think we can do much at the hooks level.

One thing it would be helpful is to show "what path is tracked" in console. But, this means developers have to understand what is "tracking by proxies".

We have to decide one important point, whether this hook is for beginners or not.

I have assumed this hook is for beginners from the start. If this is the case, I have a crazy idea. We can make the hook so that it will never forget the tracked path by default. That will erase the second caveat. In such a case, I could add an functionality to opt-out the default behavior (meaning, to clear the tracked path in each render) for optimization, or just encourage the use of useSelector for more (predictable) performance.

Feedback welcome, including request for clarification.


On second thought, I think what everyone would expect for react-redux is performance and predictability. In such a case, I don't take the crazy idea, and seek/improve debugability of "tracking".

@dai-shi
Copy link
Contributor Author

dai-shi commented Feb 21, 2020

Let me summarize my thoughts about caveats with possible ideas to mitigate them.

Caveat 1: Proxied states are referentially equal only in per-hook basis

Unless we use custom hooks, it's not very likely to be misused (and we could detect it by eslint rules.)
With custom hooks, it's hard to detect.
In either case, we would need to explicitly state that the tracked state is special.

Caveat 2: An object referential change doesn't trigger re-render if an property of the object is accessed in previous render

This is a pitfall that's hard to find if one falls into.
Per my previous comment, we should add some debugging information so that developers can "see" how it is tracking, which should allow finding pitfalls.

Caveat 3: Proxied state shouldn't be used outside of render

As I said, there are several paths. For this one, I need more feedback.
I shouldn't have said "shouldn't be used", the fact is it's not uncertain what would happen.
Let's just focus on two separate approaches:

  1. We allow passing proxied state to dispatch and unwrap it before wrapping with a new proxy (as far as I remember, this is how proxyequal works)
  2. We create another useDispatchForTrackedState, which unwraps proxied state before passing to dispatch. (Oh, but this doesn't work if we dispatch a thunk function with state in closure.)

@dai-shi
Copy link
Contributor Author

dai-shi commented Mar 1, 2020

I've made a few improvements.

  • useTrackedState can now show tracked path list with useDebugValue
    • This doesn't directly solve the caveats, but it will help finding issues, so hopefully somewhat mitigated.
  • useTrackedState always tries to unwrap the previous proxy before wrapping with a new proxy
    • Basically, this will avoid multiple wrapping, which was a major issue (leading memory leaks) in the previous caveat 3.
    • With this change, passing tracked state to dispatch and reducer should be fine as long as there are no middlware...
    • Some middleware may conflict with this behavior, but nothing is confirmed so far.
  • Docs are updated accordingly

@theKashey theKashey mentioned this pull request Oct 28, 2020
3 tasks
@dai-shi
Copy link
Contributor Author

dai-shi commented Dec 17, 2020

It turns out that this feature can be built on top of useSelector.
dai-shi/react-tracked#71
I still think it would be nice to support it natively in react-redux,
but this add-on usage would also be nice and it works.

@artem-malko
Copy link

@markerikson @dai-shi hi!)

I'd like to know, why this pull request was not merged?) Looks like useful feature. Di you need any help with it?

@dai-shi
Copy link
Contributor Author

dai-shi commented Sep 14, 2021

@artem-malko react-tracked offers the same capability opt-in. If you are interested in it, check the (naive) example: https://react-tracked.js.org/docs/tutorial-redux-01

Also, check out the proxy-memoize in the redux docs for similar (not the same) capability as selectors.

What we can do (help) is to show some use cases, how powerful these approaches are, and possible limitations.

@markerikson
Copy link
Contributor

Given that we're working on v8 right now and it's all been migrated to TS, and that proxy-memoize now exists, I think it might be time to close this PR.

I'm still open to the idea of possibly adding useTrackedState, I'm just not entirely sure it's necessary or that there's enough actual benefit.

@dai-shi , thoughts?

@dai-shi
Copy link
Contributor Author

dai-shi commented Sep 14, 2021

Yeah, I guess proxy-memoize comes with less caveats, and fits well with the current (react-)redux coding style. useTrackedState would still be worth with simpler api, and one can use react-tracked.

I close this PR with two alternatives.

react-tracked

https://react-tracked.js.org has createTrackedSelector to support react-redux.
We can create the same useTrackedState implemented in this PR, from useSelector.
(btw, react-tracked does the same with useContextSelector for React Context.)

import { useSelector } from 'react-redux';
import { createTrackedSelector } from 'react-tracked';

const useTrackedState = createTrackedSelector(useSelector);

const Component = () => {
  const { item: { name, price } } = useTrackedState();
  return <>{name}: {price}</>
}

proxy-memoize

Redux docs: https://redux.js.org/usage/deriving-data-selectors#proxy-memoize

import { useSelector } from 'react-redux';
import memoize from 'proxy-memoize';

const selectItem = memoize(state => ({ name: state.item.name, price: state.item.price }))

const Component = () => {
  const { name, price } = useSelector(selectItem);
  return <>{name}: {price}</>
}

@dai-shi dai-shi closed this Sep 14, 2021
@artem-malko
Copy link

Thx for the info)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants